-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Submitter fetches pending transactions independently per chain #2376
Conversation
WalkthroughThe recent updates focus on enhancing chain ID management in Ethergo by introducing the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Deploying sanguine-fe with Cloudflare Pages
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (5)
- ethergo/submitter/db/mocks/service.go (1 hunks)
- ethergo/submitter/db/service.go (2 hunks)
- ethergo/submitter/db/txdb/store.go (3 hunks)
- ethergo/submitter/db_test.go (1 hunks)
- ethergo/submitter/queue.go (3 hunks)
Additional comments: 10
ethergo/submitter/db/service.go (2)
- 8-8: The addition of the
math/big
package is appropriate given the use of*big.Int
in the new methodGetChainIDsByStatus
.- 39-40: The new method
GetChainIDsByStatus
is a significant addition. It's crucial to ensure that the method's implementation in the corresponding implementation files (e.g.,store.go
) correctly handles the logic for fetching distinct chain IDs based on the given address and status. This method enhances the granularity of transaction fetching, aligning with the PR's objectives.ethergo/submitter/queue.go (4)
- 6-8: The addition of
math/big
,sync
, andtime
packages supports the new functionality for handling transactions withbig.Int
chain IDs and concurrent processing.- 64-66: Fetching distinct chain IDs for pending transactions using
GetChainIDsByStatus
is a key improvement. It ensures that transactions are fetched and processed per chain, adhering to intra-chain limits. Proper error handling is observed here, which is crucial for robustness.- 69-73: Converting
*big.Int
chain IDs toint64
for logging purposes is a sensible approach. However, ensure that this conversion does not lead to any loss of information, especially for very large chain IDs that might exceed theint64
range.- 77-93: The concurrent processing of transactions per chain ID is a significant enhancement. It leverages goroutines for parallel processing, which can improve the efficiency of transaction processing. Ensure that the
sync.WaitGroup
is correctly used to wait for all goroutines to complete.ethergo/submitter/db_test.go (1)
- 163-207: The addition of
TestGetChainIDsByStatus
is crucial for verifying the new functionality. It appears to adequately test the retrieval of chain IDs by status, ensuring that the implementation behaves as expected. Consider adding more test cases to cover edge cases, such as no transactions matching the status or transactions across a wide range of chain IDs.ethergo/submitter/db/mocks/service.go (1)
- 65-93: The addition of the mock function
GetChainIDsByStatus
is essential for unit testing components that depend on theService
interface. It correctly mirrors the signature of the actual method, allowing for effective mocking of behavior in tests. Ensure that tests utilizing this mock properly simulate various return scenarios to cover different edge cases.ethergo/submitter/db/txdb/store.go (2)
- 8-8: The addition of the
math/big
package is necessary for handling chain IDs as*big.Int
, aligning with the changes made in this PR.- 357-383: The implementation of
GetChainIDsByStatus
instore.go
is a key part of this PR's objective to fetch transactions with awareness of individual chains. This method's implementation seems to correctly fetch distinct chain IDs for a given address and status. Ensure that the method is thoroughly tested, especially for cases with a large number of transactions and chain IDs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- ethergo/submitter/db/service.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- ethergo/submitter/db/service.go
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2376 +/- ##
===================================================
+ Coverage 46.89341% 46.89678% +0.00336%
===================================================
Files 399 399
Lines 30548 30565 +17
Branches 83 83
===================================================
+ Hits 14325 14334 +9
- Misses 14695 14701 +6
- Partials 1528 1530 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- ethergo/submitter/db/txdb/store.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- ethergo/submitter/db/txdb/store.go
Description
Addresses issue where the
GetTXS()
query for pending transactions enforces inter-chain limit instead of intra-chain limit. Instead of introducing an overly complex raw SQL query, we add aGetChainIDsByStatus()
helper to the submitter db and get transactions with individual queries per chain.Summary by CodeRabbit